Skip to content

perf(relay): index channel-id lookups and skip trace-only reads - #4647

Merged
tlongwell-block merged 1 commit into
block:mainfrom
jemiahw:fix/channels-id-lookup-hot-query
Aug 4, 2026
Merged

perf(relay): index channel-id lookups and skip trace-only reads#4647
tlongwell-block merged 1 commit into
block:mainfrom
jemiahw:fix/channels-id-lookup-hot-query

Conversation

@jemiahw

@jemiahw jemiahw commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Problem

SELECT id, community_id FROM channels WHERE id = ANY($1) AND deleted_at IS NULL is the top Load by waits (AAS) on the Buzz Postgres writer. Two independent causes compound, and both are fixed here.

1. No index can serve it

channels is PRIMARY KEY (community_id, id), and every secondary index leads with community_id:

Index Columns
(primary key) (community_id, id)
idx_channels_nip29_group (community_id, nip29_group_id)
idx_channels_dm_hash (community_id, participant_hash)
idx_channels_community_type (community_id, channel_type)
idx_channels_community_visibility (community_id, visibility)
idx_channels_created_by (community_id, created_by)
idx_channels_ttl_expiry (ttl_deadline) (partial)

The two tenant-independent lookups carry no community_id predicate — deliberately:

  • Db::communities_of_channelsWHERE id = ANY($1) AND deleted_at IS NULL
  • Db::community_of_channelWHERE id = $1 AND deleted_at IS NULL

That independence is load-bearing, not an oversight: projecting a row's true owning community regardless of the fetch query's WHERE clause is what makes Inv_NonInterference non-vacuous. If the fetch ever dropped its tenant scoping, this lookup would still report the real label and the checker would catch the mismatch.

But a composite btree is only usable when its leading column is constrained, so neither query can use the primary key, and nothing else leads with id. Both sequentially scan channels on every call.

2. In production the result is discarded

Both call sites feed record_read_message_rows / record_read_by_id_rows, which call tracer.record(...). Production binds NoopTracer (crates/buzz-relay/src/state.rs), whose record body is empty.

The existing guard tests trace_state, which is Some for every well-formed request — it only goes None on malformed pubkey bytes. So the scan ran on the hot read path and its output was dropped. This is the classic eager-argument bug: log.debug("..." + expensiveCall()) with no isDebugEnabled() check.

3. Multiplied per filter

The non-search call site sits inside the phase-3 per-filter loop, so a REQ carrying N filters performed N sequential scans of channels before responding.

Changes

Tracer::enabled() — a capability check on the trait (the isDebugEnabled() of this seam), defaulting to true. NoopTracer overrides it to false, and both emitters in req.rs now gate on it, skipping the trace-only DB read entirely in production.

migrations/0027_channels_id_lookup_index.sql

CREATE INDEX IF NOT EXISTS idx_channels_id_live
    ON channels (id) INCLUDE (community_id)
    WHERE deleted_at IS NULL;
  • INCLUDE (community_id) — both queries select exactly (id, community_id), so this is covering and can be served index-only.
  • Partial on deleted_at IS NULL — matches both predicates exactly, excludes soft-deleted history, and lets Postgres skip the recheck.
  • Not UNIQUE. id alone is not unique in this table — command_executor.rs documents that community_of_channel(channel_id) is ambiguous because the same channel id can appear under more than one community. A unique index would encode a false constraint and fail to build on any database already holding such a pair.

Worth keeping the index even though fix #1 removes the production caller: it still runs under conformance, and community_of_channel has the same problem on its own paths.

schema/schema.sql — mirrored, since a test asserts desired-state parity.

Conformance is unchanged

This is the part worth reviewing closely. Under a real tracer enabled() returns true and every emit happens exactly as before — the gate only skips building emit inputs when nothing observes them, never an emit that would otherwise have been made. The coverage-breach guard stays non-vacuous.

CountingTracer forwards enabled() to its inner tracer rather than inheriting the true default. Both directions matter and both fail silently:

  • inheriting true over a NoopTracer would keep the overhead this PR removes;
  • hardcoding false over a live tracer would suppress the emits whose absence EmitGuard reports as ImplBug — masking real breaches behind expected ones.

Covered by a new regression test, counting_tracer_delegates_enabled_to_inner, which asserts delegation in both directions.

Verification

  • cargo check -p buzz-conformance -p buzz-relay — clean
  • cargo clippy --all-targets — clean, zero warnings
  • cargo test -p buzz-conformance — 6/6
  • cargo test -p buzz-relay --lib conformance — 11/11
  • cargo test -p buzz-db --lib migration — 7/7
  • just test-unit (pre-push) — green

Migration-count assertions in crates/buzz-db/src/migration.rs were bumped 26 → 27, with content assertions for 0027 following the existing per-migration pattern (including a guard that it never becomes UNIQUE).

Open questions for reviewers

  1. Lock strategy. Built without CONCURRENTLY, following migration 0004's precedent, because sqlx runs each migration inside a transaction and CREATE INDEX CONCURRENTLY cannot run in one. This takes a brief SHARE lock on channels (blocks writes, not reads) — small relative to events, but an operator preferring zero write-blocking can pre-build it by hand and IF NOT EXISTS makes the migration a no-op. I could not confirm whether sqlx 0.9 supports a -- no-transaction directive; if it does, that may be preferable.

  2. Diagnosis is static. This comes from reading the source, not from EXPLAIN against the live database. Worth confirming with EXPLAIN (ANALYZE, BUFFERS) on the writer before/after — that also sizes the win by revealing the real table size and row counts.

  3. Expected impact scales with average filters-per-REQ, which I did not measure. pg_stat_statements ordered by total_exec_time would confirm this query drops off the top and show whether anything else is scanning the same way.

`SELECT id, community_id FROM channels WHERE id = ANY($1) AND deleted_at
IS NULL` is the top "Load by waits (AAS)" on the writer. Two independent
causes, both fixed here.

No index can serve it. `channels` is PRIMARY KEY (community_id, id) and
every secondary index leads with community_id, but the tenant-independent
lookups (`Db::communities_of_channels`, `Db::community_of_channel`) carry
no community_id predicate — deliberately, since projecting a row's true
label independently of the fetch query's WHERE clause is what makes
Inv_NonInterference non-vacuous. A composite btree needs its leading
column constrained, so both queries sequentially scan `channels` on every
call. Migration 0027 adds a covering partial index on (id) INCLUDE
(community_id) WHERE deleted_at IS NULL, serving both index-only. Not
UNIQUE: `id` alone is not unique, as command_executor.rs documents.

The result was also discarded in production. Both callers feed
`record_read_*_rows` -> `tracer.record(...)`, and production binds
`NoopTracer`, whose `record` is empty. The existing `if let Some(...)`
guard tests `trace_state`, which is `Some` for every well-formed request,
so the scan ran and its output was dropped — once per filter for the
non-search lane, inside the phase-3 loop. `Tracer` grows an `enabled()`
capability check (the `log.isDebugEnabled()` of this seam) that
`NoopTracer` overrides to `false`, and both emitters now gate on it.

Conformance behaviour is unchanged: a real tracer reports `enabled() ==
true` and every emit happens exactly as before, so the coverage-breach
guard stays non-vacuous. `CountingTracer` forwards `enabled()` to its
inner tracer rather than inheriting the `true` default — inheriting it
would keep the overhead over a `NoopTracer`, and hardcoding `false` would
suppress emits the `EmitGuard` reports as `ImplBug`. Covered by
`counting_tracer_delegates_enabled_to_inner`.

Built without CONCURRENTLY, per 0004's precedent: sqlx runs each
migration in a transaction. Takes a brief SHARE lock on `channels`;
operators wanting zero write-blocking can pre-build the index
concurrently by hand, and IF NOT EXISTS makes the migration a no-op.

Signed-off-by: Jemiah Westerman <jemiah@squareup.com>
@tlongwell-block
tlongwell-block merged commit bc9e652 into block:main Aug 4, 2026
33 checks passed
veltri-23 added a commit to veltri-23/buzz that referenced this pull request Aug 4, 2026
PR block#4647 merged as 0027_channels_id_lookup_index.sql, so the embedded
migrator would have two version-27 migrations. Whichever merged second
breaks the sqlx checksum (VersionMismatch(27)) at boot with
BUZZ_AUTO_MIGRATE=true. Renumber to 0028 and bump the count assertion
27 -> 28.

Signed-off-by: Hunter Veltri <veltrifinancial@gmail.com>
wpfleger96 pushed a commit that referenced this pull request Aug 4, 2026
…-enabled

* origin/main:
  Dock Buzz Term within channel workspace (#4724)
  perf(relay): index channel-id lookups and skip trace-only reads (#4647)
  fix(agents): canonicalize stale persona harness pins (#4631)
  Refine community invite links (#4734)

Signed-off-by: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
wpfleger96 added a commit that referenced this pull request Aug 4, 2026
* commit 'ce3cf3cd2': (76 commits)
  Polish Huddle voice controls (#4694)
  fix(local-archive): default both archive settings to enabled (#4750)
  fix(mobile): stop oversized read-state retry loop (#4595)
  fix(desktop): close reconnect gaps that previously required CMD+R (#4737)
  Dock Buzz Term within channel workspace (#4724)
  perf(relay): index channel-id lookups and skip trace-only reads (#4647)
  fix(agents): canonicalize stale persona harness pins (#4631)
  Refine community invite links (#4734)
  feat(desktop): persist sidebar observed-unread across webview reload (#3976)
  feat(desktop): surface config diff in restart-required badge (#3637)
  Polish sidebar unread hierarchy (#4573)
  fix(desktop): show cached display names on startup (#3317)
  docs(acp): explain per-channel session model in base prompt (#4729)
  docs(nip-am): normative amendment — cache SHOULD/MUST + pricingIdentity + consumer cost guidance (#4632)
  Remove blur from Welcome composer guidance (#4691)
  Refine desktop timeline activity presentation (#4582)
  Defer desktop media uploads until send (#4522)
  fix(desktop): stop clipping focus ring on channel intro action cards (#2392) (#4374)
  Polish mobile inbox and media flows (#4512)
  feat: ship Buzz Term (#4347)
  ...

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
shellz-n-stuff added a commit to shellz-n-stuff/buzz that referenced this pull request Aug 4, 2026
…gent-instructions

* origin/main: (30 commits)
  feat: Buzz entity links — rich preview cards + in-app navigation for repos, PRs, and issues (block#4695)
  fix(desktop): serialize tray channel actions for frontend (block#4762)
  chore(release): release Buzz Desktop version 0.5.5 (block#4788)
  feat(projects): support multiple repositories (block#4671)
  fix(ci): make desktop cache test version agnostic (block#4791)
  fix(desktop): widen post-Enter timeouts in empty-edit-delete spec (block#4792)
  fix(desktop): wait for terminal frame before splash (block#4781)
  fix(desktop): integer-align custom reaction emoji (block#4779)
  Polish Huddle voice controls (block#4694)
  fix(local-archive): default both archive settings to enabled (block#4750)
  fix(mobile): stop oversized read-state retry loop (block#4595)
  fix(desktop): close reconnect gaps that previously required CMD+R (block#4737)
  Dock Buzz Term within channel workspace (block#4724)
  perf(relay): index channel-id lookups and skip trace-only reads (block#4647)
  fix(agents): canonicalize stale persona harness pins (block#4631)
  Refine community invite links (block#4734)
  feat(desktop): persist sidebar observed-unread across webview reload (block#3976)
  feat(desktop): surface config diff in restart-required badge (block#3637)
  Polish sidebar unread hierarchy (block#4573)
  fix(desktop): show cached display names on startup (block#3317)
  ...

Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dependency Dashboard

3 participants